Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Migrate from gulp and webpack, to vite #2815

Draft
wants to merge 5 commits into
base: devel
Choose a base branch
from
Draft

Migrate from gulp and webpack, to vite #2815

wants to merge 5 commits into from

Conversation

pygeek
Copy link
Contributor

@pygeek pygeek commented Sep 8, 2024 •
edited
Loading

  • Simplify and speed up the build/dev process by replacing Gulp and Webpack with Vite.
  • Additional changes in this PR need to be made to enable HMR — an additional lint rule has been added to support this.

@pygeek pygeek changed the title Gulp to vite Migrate from gulp and webpack, to vite Sep 8, 2024
anoek
anoek reviewed Sep 8, 2024
View reviewed changes
src/vite.config.ts Outdated Show resolved Hide resolved
@anoek
Copy link
Member

anoek commented Sep 8, 2024

Is there a particular reason for the vite.config.js to reside in src/? Our other config files are up a directory, would it be fine to reside there too?

@pygeek
Copy link
Contributor Author

pygeek commented Sep 8, 2024 •
edited
Loading

I initially settled on that approach, because it was the fastest to prototype the migration—I was having issues with the pre-existing typescript setup. I've updated the code with my preferred setup (index.html + vite.config.js moved outside of src) now that I've worked out the bugs and made changes to the typescript + package setup.

Context: Vite has a constraint where the vite.config.js must live alongside the index.html.

@pygeek pygeek mentioned this pull request Sep 10, 2024
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 11, 2024 •
edited
Loading

Uffizzi Ephemeral Environment deployment-56139

☁️ https://app.uffizzi.com/github.com/online-go/online-go+com/pull/2815

📄 View Application Logs etc.

What is Uffizzi? Learn more!

benjaminpjones
benjaminpjones reviewed Sep 11, 2024
View reviewed changes
Copy link
Contributor

@benjaminpjones benjaminpjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think a switch to Vite would be exciting. It seems to be the preferred modern build tool for React and other frameworks :)

index.html
} catch (error) {
console.warn(error);
console.log("No WASM support detected, score estimator falling back to ASM.js mode");
script.src = "/OGSScoreEstimator/OGSScoreEstimator-0.7.0.asm.js";
script.src = "//localhost:8080/OGSScoreEstimator/OGSScoreEstimator-0.7.0.asm.js";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work in other environments? (i.e. user clients)

Copy link
Contributor Author

@pygeek pygeek Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Values were temporarily hardcoded for prototyping.

index.html
@@ -1,19 +1,18 @@
<!doctype html>
<html lang="{{PAGE_LANGUAGE}}">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't Vite also have the ability to use variables in html?

https://vitejs.dev/guide/env-and-mode.html#html-env-replacement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was only done for rapid prototyping.

package.json
@@ -9,19 +9,15 @@
"npm": ">=8.5.0"
},
"scripts": {
"dev": "supervisor -w Gulpfile.js,webpack.config.js,tsconfig.json supervisor -w Gulpfile.js -x gulp --",
"dev": "npx tsx server.ts",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reload on change is definitely an important feature for devs. (Is this what you're referring to with the HMR updates needed?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vite supports live reload by default. However, with HMR (Hot Module Replacement) the specific module which was updated is reloaded, rather than the entire app. https://webpack.js.org/concepts/hot-module-replacement/

In order to get this working, I have to make updates to a few components. Once this is done, Vite will automatically enable HMR. Related eslint rule: https://github.com/ArnaudBarre/eslint-plugin-react-refresh

anoek
anoek reviewed Sep 11, 2024
View reviewed changes
Copy link
Member

@anoek anoek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty excited for this, I've looked into it in the past but it was a bit daunting, thanks for taking it on.

FTR there's some webpack usage within our Makefile.production that will need to be replaced with the appropriate vite equivalents, probably most notably is the production builds:

online-go.com/Makefile.production

Lines 125 to 132 in 4b40715

dist/ogs.min.js: dist/ogs.js
npm run webpack -- --mode=production --optimization-minimize --devtool=source-map
dist/vendor.min.js: dist/vendor.js
npm run webpack -- --mode=production --optimization-minimize --devtool=source-map
dist/ogs.$(VERSION).css:
npm run gulp min_styl

package.json
"spellcheck": "cspell \"src/**/*.{ts,tsx}\"",
"minify-index": "gulp minify-index --silent",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI we do use this targe during deployment, https://github.com/online-go/online-go.com/blob/devel/Makefile.production#L48

@github-actions GitHub Actions
Copy link

This pull request has been marked stale and will be closed soon without further activity. Please update the PR or respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the stale Issues or PRs that have been open for a long time with no activity label Nov 13, 2024
@pygeek
Copy link
Contributor Author

pygeek commented Nov 15, 2024

This pull request has been marked stale and will be closed soon without further activity. Please update the PR or respond to this comment if you're still interested in working on this.

still working on this

@github-actions github-actions bot removed the stale Issues or PRs that have been open for a long time with no activity label Nov 16, 2024
@github-actions GitHub Actions
Copy link

This pull request has been marked stale and will be closed soon without further activity. Please update the PR or respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the stale Issues or PRs that have been open for a long time with no activity label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjaminpjones benjaminpjones benjaminpjones left review comments

@anoek anoek anoek left review comments

Assignees
No one assigned
Labels
stale Issues or PRs that have been open for a long time with no activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pygeek @anoek @benjaminpjones